-
-
Notifications
You must be signed in to change notification settings - Fork 266
❗BREAKING CHANGE❗️Ensure compatibility with graphql-php v15 #953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ec0da08
to
bf2668a
Compare
bf2668a
to
d8469da
Compare
composer.json
Outdated
@@ -38,9 +38,9 @@ | |||
"ext-json": "*", | |||
"illuminate/contracts": "^6.0|^8.0|^9.0", | |||
"illuminate/support": "^6.0|^8.0|^9.0", | |||
"laragraph/utils": "^1", | |||
"laragraph/utils": "1.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"laragraph/utils": "1.4.1", | |
"laragraph/utils": "dev-graphql-php-15", |
Should be the easier temporary fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my… I never connected the dots, that what follows dev-…
can be any qualifier, even a branch name 🤦🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks, everything is green!
A later commit adds support back for our own error classes.
…y for our own error classes
The ability to internally prepend the graphql-php namespace was removed [1]. [1] webonyx/graphql-php@d36e809#diff-fa0403de31548d680d863a100fdc906ec53696abb9d1e8b0cbc92020b7cce9a2L255
Until we figure out what the replacement for the getter is
…ReadOnly` have been dropped in favour of the public properties
5d06f71
to
c01f88d
Compare
This reverts commit 86f2ded.
# Conflicts: # phpstan.neon.dist
da4bcd1
to
82d13d1
Compare
# Conflicts: # CHANGELOG.md # composer.json
3bfa578
to
82c14c1
Compare
82c14c1
to
503a966
Compare
We are having this issue when running tests on PHP 8.2, I guess because rebing is still using the older webonyx lib?
|
Yes, this sounds about right; and this PR would fix this. |
v15 has been released |
I have been testing these changes against our tests. I'm running into an exception when printing the generated schema via the upstream GraphQL\Utils\SchemaPrinter::doPrint. I've made a test case and a solution, but not sure if that's the best solution: |
Cool find @sforward ! Can you make a PR against master with just the test? Assuming it would just work, next time I rebase this PR I'll then fix it considering your suggestion in the commit. WDYT? |
Sure, I've created a MR with the test! |
# Conflicts: # .github/workflows/tests.yml # CHANGELOG.md # composer.json # phpstan-baseline.neon # phpstan.neon.dist # tests/Unit/GraphQLTest.php
This seems to be related to the recent change in webonyx/graphql-php#1303 . If I revert these changes in Not yet sure what the expectation is here. |
@sforward I came up with an alternate fix: it seems the change in graphql-php now expects the lazy type loader to also answer for the top level I've added them in the latest commit but also opened webonyx/graphql-php#1336 to see if that's actually intentional. |
Summary
See webonyx/graphql-php#1231
❗ Merging this will require a major version bump due to all around incompatible changes forced by graphql-php ❗
TODO
\Rebing\GraphQL\Tests\Unit\ConfigTest::testSecurity
They current aren't due to the composer.json hack
Type of change:
Checklist:
composer fix-style